Implement Additional Feedback on PR for SR-11580 #240
Conversation
| return potentialFloatingValue! | ||
| } | ||
|
|
||
| fileprivate var potentialFloatingValue: Double? { |
There was a problem hiding this comment.
One benefit of this PR is that this property is no longer needed.
| XCTAssertNil(literalExpr) | ||
| XCTAssertNil(expectedValue) | ||
| } | ||
| } |
There was a problem hiding this comment.
git diff called out this absence of a terminating newline, so I added one.
There was a problem hiding this comment.
After a quick scan of the PR, I’ve got two directions in which this could go:
1. The properties don’t need to be optional at all
AFAICT, the ExpressibleByFloatLiteral protocol has a requirement for an initialiser that takes a ExpressibleByFloatLiteral.FloatLiteralType. That type needs to conform to _ExpressibleByBuiltinFloatLiteral (see here). However, the only types that conform to _ExpressibleByBuiltinFloatLiteral according to here and here are Float16, Float, Double and Float80. So we cannot have a floating point literal that doesn’t fit in a Float80.
I know that this is a semantic property, but I would say that for the sake of usability, it would be alright to harness this semantic property. What do you think @xwu and @akyrtzi?
For Int the argument is similar with the only difference that we need to take into account signed and unsigned integers (Int and UInt being the biggest types). But I think we can tackle that problem by returning a type like the following from the intLiteralValue property and using either the signed or unsigned case depending on whether the literal starts with a - or not:
enum IntLiteralValue {
case signed(Int)
case unsigned(UInt)
var signedValue: Int? { /* … */ }
var unsignedValue: Int? { /* … */ }
}2. We don’t need the must_uphold_invariant thing anymore
If we decide that we want to make the floatLiteralValue and intLiteralValue properties optional, we don’t need the entire must_uphold_invariant thing anymore.
In fact, as far as it’s implemented right now, there are currently two issues with the invariant:
a) It is no longer true that every syntactically valid literal upholds its invariant because it might not fit into the property’s type
b) We cannot set the contents of a float literal to anything that’s larger than the property’s type.
That would require that we change the parser to make overflowing literals syntactically invalid. But I don't think that is appropriate, I'd recommend that we adhere to what is valid syntax and avoid bringing in semantic/code-generation concepts. |
|
It'll take me until the weekend to write up my feedback for this PR--sorry for the delay. Overall, it's going to be a hair more complicated than what's presented here. @ahoppen Both We can very much have a valid float literal that doesn't fit in a For integer literals, The purpose of all of this work (including any future overhaul of |
|
On second thought, I quite agree with @xwu and @akyrtzi that we shouldn’t rely on semantic functionality. AFAICT it should be sufficient if we make the properties optional and remove the I’m sorry for all the work you have put into this, @vermont42, only so it needs to be scrapped now… |
|
@xwu would you be fine with making these simple changes for now (make the properties optional and remove |
There was a problem hiding this comment.
Sorry for the delay. Yes, I do have a comment on what an ideal API might be, but that shouldn't hold up this PR because it could be addressed further down the road.
However, I also have some comments on the correctness of the implementation, and I think those are pressing because they affect the raison d'être of this API: Users want to know, and this API purports to tell us, what value is represented by a particular literal expression. If it gives false positives and/or false negatives, then it does not serve that purpose, and it would not be a usable API to use.
As it happens, I think my comment on the API design is actually fairly straightforward to address, though not pressing, but my comments on the correctness of the implementation are quite hard to address but very much pressing. In fact, the potential crash is actually the least urgent in my view, as in that circumstance at least execution does not proceed with a misleading result; it's where the API returns an inaccurate result without crashing that really makes me worry. I really don't mind if we don't address the entirety of my comment on the API design, but if addressing the other issues prove to be unworkable in the short term, perhaps in the interim we could revert the previous PR?
|
|
||
| fileprivate var potentialFloatingValue: Double? { | ||
| var floatLiteralValue: Double? { | ||
| let floatingDigitsWithoutUnderscores = floatingDigits.text.filter { |
There was a problem hiding this comment.
Unless there is other validation of text of which I'm unaware, it's not sufficient simply to drop all underscores. There are rules surrounding where underscores may appear, and there are combinations of underscores that are invalid. For example, 1.0e_3 is not permitted, because an underscore in the floating point exponent is not allowed. Simply dropping underscores means that you will have false positives where floatLiteralValue returns a non-nil result but in fact the literal represented is invalid.
The rules for where underscores are allowed (where, how many consecutively, etc.) aren't easily found and, even when found, may be inaccurate. For instance, according to PEP 515 (which added underscore separators to numeric literals in Python and surveyed the rules in other languages), Swift documentation says that any number of underscores are allowed between digits, but in fact they are allowed in Swift at the end of literals after the last digit as well.
This will likely require either experimentation or a careful reading of the Swift compiler's code. If you undertake this work, please do document it somewhere for posterity.
Similar issues may apply to integer literals too, although to a lesser degree because the syntax is less complex. Even if there turn out to be no such issues, it will be good to undertake the exercise of proving that and documenting it.
| let floatingDigitsWithoutUnderscores = floatingDigits.text.filter { | ||
| $0 != "_" | ||
| } | ||
| return Double(floatingDigitsWithoutUnderscores) |
There was a problem hiding this comment.
Using the String-to-Double conversion API may produce both false positives and false negatives (again, unless there is other validation of text of which I'm unaware); in other words, there will be valid literals for which this function will return nil, and invalid literals for which this function returns a value. The reason for this is that Swift's rules for numeric literals diverge from what's allowed for C's numeric literals, while conversions to and from String values use (or at least follow more closely) C's rules, and it's too late to change that behavior because it will break existing code.
As an example, .5 is not a valid Swift float literal, but ".5" is a string that can be converted to a floating-point value using Double(".5"). The reason for this particular divergence is that Swift has a leading-dot member syntax.
There are other examples listed here that will likely require some experimentation to ensure are still applicable and as described. It is likely that I have missed some of the differences too (for instance, I never did list the differences in behavior when a float literal is too large to be represented by the desired type versus a string).
Again, similar issues may apply to integer literals, and even if there turn out to be no such issues, it would be important to prove and document that.
| var integerValue: Int { | ||
| return potentialIntegerValue! | ||
| var isValid: Bool { | ||
| floatLiteralValue != nil |
There was a problem hiding this comment.
I would suggest removal of isValid altogether for now; whether a float literal is a valid or invalid literal has no relationship to whether it can be converted to a value of any particular type, for reasons we discussed earlier.
It may be, however, that in your experimentation regarding the issues above, you end up having a sufficiently complete implementation validating Swift's syntactic rules surrounding literals that you can implement isValid without reference to conversion to any particular floating-point type. In that case, then by all means include an isValid member that checks for validity of the literal.
The same comment applies to the other isValid for integer literals.
|
|
||
| fileprivate var potentialIntegerValue: Int? { | ||
| public extension IntegerLiteralExprSyntax { | ||
| var integerLiteralValue: Int? { |
There was a problem hiding this comment.
OK, so here's my one comment on API design. I think users will want to know what value is represented by their numeric literal expression for types other than the default Int and Double. It would be very reasonable to ask what's the UInt32 value represented by a certain literal, for instance.
Therefore, we can use a (generic) method instead of a property to allow the user to pass a variable indicating the desired type:
func integerLiteralValue<T>(ofType _: T.Type) -> T
where T: ExpressibleByIntegerLiteral { ... }Users could then write: literalExpr.integerLiteralValue(ofType: UInt32.self). Now, it's likely that users will most often want a value of the default literal types, so it's reasonable to add that default so that users don't have to type it out every time:
func integerLiteralValue<T>(ofType _: T.Type = IntegerLiteralType) -> T
where T: ExpressibleByIntegerLiteral { ... }Now, the use site simply differs from what you have currently in this PR by a pair of parentheses. Pretty good.
However, if you were to try to implement this API, you'd find that you actually need to constrain T a bit further in order to use the String conversion APIs. It's tempting to write where T: ExpressibleByIntegerLiteral, T: LosslessStringConvertible, but in fact this raises a problem: there's no semantic guarantee that the string format from which a type can be losslessly converted is the same as the literal format (and in fact, they don't, as described above, but at least it's somewhat close for the built-in types). But in practice, we might be able to get away with it.
However, you'd run into another problem when it comes to integer literals, because if you want to pass the radix argument, you'd need to constrain to T: FixedWidthInteger instead of T: LosslessStringConvertible. This is fine, but floating-point types are expressible by integer literals (this is another way in which literals and numeric types differ from their C counterparts) but they are not fixed-width integer types; it'd be difficult to make it possible to convert from binary or octal integer literals to floating-point types without implementing your own conversions from scratch here. This is certainly not necessary for this PR and likely out of scope for this project entirely, but if we ignore floating-point types and integer literals, you can get 80% of the way just by making a few adjustments to the API design.
| testFloatValue(text: "0xaffab1e.e1fP-2", expectedValue: 0xaffab1e.e1fP-2) | ||
| testFloatValue(text: "🥥", expectedValue: nil) | ||
| // The following results in a valid FloatLiteralExprSyntax, but because Double.greatestFiniteMagnitude | ||
| // is 1.7976931348623157e+308, floatLiteralValue should be nil. |
There was a problem hiding this comment.
It should not be nil. If you write let x = 1.7976931348623157e+309, you get an actual Double which is infinity. This is one of those divergences I discuss above between the string conversion APIs and the float literal APIs.
There was a problem hiding this comment.
The last commit was about removing must_uphold_invariant and verifying that unit tests still pass. I have not yet explored additional changes.
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
5 participants
After the PR for SR-11580 merged, there was consensus, based on feedback by @xwu, that the new convenience methods
floatingValueandintegerValue, should be optional in order to prevent potential crashes. This PR implements that change.This PR also implements new names, suggested by @xwu, for
floatingValueandintegerValue:floatLiteralValueandintegerLiteralValue. These names LGTM.@xwu also stated, "Ideally, [the convenience methods] would be generic over
T: ExpressibleByIntegerLiteralandT: ExpressibleByFloatLiteralso users can get the value represented in any type they want." I am not a generics power user, but I am familiar with their use in the context of, for example, making aNodetype generic over anElementtype. I was unable to translate this understanding of the purpose and use of generics into an implementation of @xwu's suggestion. I would welcome a suggested new signature forfloatLiteralValueorintegerLiteralValue.